Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix actorcompiler target in CMake add_flow_target #11722

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sepeth
Copy link
Contributor

@sepeth sepeth commented Oct 21, 2024

I noticed this when working on a minimal Linux environment.

This also fixes #11595 - if Unix Makefiles is chosen for CMake builds, build was failing with:

make[2]: *** No rule to make target 'actorcompiler.exe', needed by 'flow/ActorCollection.actor.g.cpp'.  Stop.

I suspect it could have been a problem for Ninja as well since the issue was due to race condition, but probably it didn't happened so far for other unknown factors.

See this example in CMake add_custom_command documentation:

https://cmake.org/cmake/help/latest/command/add_custom_command.html#example-generating-files-for-multiple-targets

The correct target to depend on is actorcompiler for CMake to generate the right dependency order, ${actor_exe} is just a string that points to the location of the actor compiler. See here:

add_custom_target(actorcompiler DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/actorcompiler.exe)
set(actor_exe "${CMAKE_CURRENT_BINARY_DIR}/actorcompiler.exe")

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

This also fixes apple#11595 - if Unix Makefiles is chosen for CMake builds,
build was failing with:

```
make[2]: *** No rule to make target 'actorcompiler.exe', needed by 'flow/ActorCollection.actor.g.cpp'.  Stop.
```

I suspect it could have been a problem for Ninja as well since the issue
was due to race condition, but probably it didn't happened so far for
other unknown factors.

See this example in CMake add_custom_command documentation:

https://cmake.org/cmake/help/latest/command/add_custom_command.html#example-generating-files-for-multiple-targets

The correct target to depend on is `actorcompiler` for CMake to generate
the right dependency order, `${actor_exe}` is just a string that points to
the location of the actor compiler. See here:

https://github.com/apple/foundationdb/blob/4260bbb3c2e81531e1ec1493424544a2bea0c867/cmake/CompileActorCompiler.cmake#L26-L27
@jzhou77 jzhou77 closed this Oct 22, 2024
@jzhou77 jzhou77 reopened this Oct 22, 2024
@jzhou77 jzhou77 requested a review from xis19 October 22, 2024 00:37
@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: 11d01db
  • Duration 0:21:32
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: 11d01db
  • Duration 0:43:37
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: 11d01db
  • Duration 0:49:52
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: 11d01db
  • Duration 0:51:40
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 11d01db
  • Duration 0:57:55
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 11d01db
  • Duration 1:04:34
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: 11d01db
  • Duration 1:07:00
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building FoundationDB would fail if CMake uses Makefile
3 participants